Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add downgrade logic for branch in DocLinkService #3483

Merged

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Feb 22, 2023

Description

This is a TODO after PR.
The CoreStart.docLinks.docVersion is using pkg.branch. In 2.5 we changed it manually.
In this PR I want to add a mechanism into CI flow to prevent human error(If build team forget to change the value, which is 2.x now in current build flow, it will break down all the new doc links inside IM plugin because the link will become like https://opensearch.org/docs/2.x/api-reference/index-apis/create-index/.).

Issues Resolved

#3490 #3490

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Sorry, something went wrong.

Signed-off-by: suzhou <suzhou@amazon.com>
@SuZhou-Joe SuZhou-Joe requested a review from a team as a code owner February 22, 2023 09:51
Signed-off-by: suzhou <suzhou@amazon.com>
Signed-off-by: suzhou <suzhou@amazon.com>
@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Feb 22, 2023

Please help to backport to 2.x and main.

@joshuarrrr
Copy link
Member

joshuarrrr commented Feb 23, 2023

@SuZhou-Joe Can you make an issue for this (see https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/CONTRIBUTING.md#contributing-code)? It's a great find and definitely a bug we should fix, but I think we may want to discuss other solutions. For example:

  1. Updating the docs site routing so that 2.x URLs resolve to the latest released version
  2. Updating the logic to resolve 2.x branches such as in https://github.com/joshuarrrr/OpenSearch-Dashboards/blob/main/src/core/public/doc_links/doc_links_service.ts#L42-L45

@joshuarrrr
Copy link
Member

cc @AMoo-Miki who is keeping 👀 on all things version related.

@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Feb 23, 2023

@SuZhou-Joe Can you make an issue for this (see https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/CONTRIBUTING.md#contributing-code)? It's a great find and definitely a bug we should fix, but I think we may want to discuss other solutions. For example:

  1. Updating the docs site routing so that 2.x URLs resolve to the latest released version
  2. Updating the logic to resolve 2.x branches such as in https://github.com/joshuarrrr/OpenSearch-Dashboards/blob/main/src/core/public/doc_links/doc_links_service.ts#L42-L45

Thanks @joshuarrrr , the issue has been created and linked.
For option 1, we would like to use fixed version for specific version, for example when users are using the 2.4 version build, they should be redirect to opensearch.org/2.4/some_doc to prevent confusion when break changes happen on the API.
For option 2, yes it may work as well but I think we should put it in build layer instead of on server runtime but if you insist, I will change the implementation.

@SuZhou-Joe SuZhou-Joe mentioned this pull request Feb 23, 2023
8 tasks
Signed-off-by: suzhou <suzhou@amazon.com>

Co-authored-by: Miki <amoo_miki@yahoo.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #3483 (f0d6198) into main (02e0bc1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #3483   +/-   ##
=======================================
  Coverage   66.45%   66.46%           
=======================================
  Files        3205     3208    +3     
  Lines       61562    61602   +40     
  Branches     9497     9503    +6     
=======================================
+ Hits        40912    40943   +31     
- Misses      18353    18383   +30     
+ Partials     2297     2276   -21     
Flag Coverage Δ
Linux 66.40% <100.00%> (+<0.01%) ⬆️
Windows 66.41% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/core/public/doc_links/doc_links_service.ts 100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AMoo-Miki
AMoo-Miki previously approved these changes Feb 24, 2023
Signed-off-by: suzhou <suzhou@amazon.com>
AMoo-Miki
AMoo-Miki previously approved these changes Mar 8, 2023
@SuZhou-Joe
Copy link
Member Author

Please add backport 2.x label for this PR. This should be merged to 2.x as well.

@kavilla
Copy link
Member

kavilla commented Mar 14, 2023

Hello @SuZhou-Joe,

Thanks for opening!

From my understanding it takes the config value and automatically sets it based on the config build version if not main to which point I kind of wonder what's the point of even making branch a config value.

I believe the branch is only used within the doc links service within our repo but I'm not positive if external plugins utilize this configuration for information.

If the purpose is for the doc links service, I'd recommend handling this logic https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/public/doc_links/doc_links_service.ts#L43.

Also, I can kind of see how we build the doc links service to be very unique to the branching strategy and doc link URL path set by the organization. I think it's fine not to worry about for now but if I was to fork this and change the URL I think it should be valid to pass suffix. I should even be able to pass my own site. But like I said, that's not something for you something that we should think about.

Let me know if you have any questions!

Thanks!

branch = pkg.branch;
} else {
// package version was not parsable and branch is unusable
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this prevent the builds if they pass a non-valid doc paths pattern in the branch in the package?

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if no version can be specified after so many downgrade logic, we will prevent the build because there will be a doc link error if we continue to build.

Signed-off-by: suzhou <suzhou@amazon.com>
Signed-off-by: suzhou <suzhou@amazon.com>
This reverts commit 8f8521d.

Signed-off-by: suzhou <suzhou@amazon.com>
This reverts commit b09c3eb.

Signed-off-by: suzhou <suzhou@amazon.com>
This reverts commit 730d694.

Signed-off-by: suzhou <suzhou@amazon.com>
This reverts commit 9df87a5.

Signed-off-by: suzhou <suzhou@amazon.com>
@SuZhou-Joe SuZhou-Joe force-pushed the feature/patch_branch_when_build branch from ccfe876 to 35fff82 Compare March 21, 2023 02:58
branch = pkgBranch;
} else {
// package version was not parsable and branch is unusable
throw new Error(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
When the logic goes to here, the dashboard will display such error page. But it won't be here because dashboard will check the version with OpenSearch core and thus it will always be parsed successfully by semver.

@SuZhou-Joe SuZhou-Joe changed the title Patch pkg.branch when build Add downgrade logic for branch in DocLinkService Mar 21, 2023
Signed-off-by: suzhou <suzhou@amazon.com>
Signed-off-by: suzhou <suzhou@amazon.com>
@kavilla kavilla linked an issue Mar 21, 2023 that may be closed by this pull request
kavilla
kavilla previously approved these changes Mar 21, 2023
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

} else if (parsedBuildVersion) {
branch = `${parsedBuildVersion.major}.${parsedBuildVersion.minor}`;
} else {
branch = pkgBranch;
Copy link
Member

@abbyhu2000 abbyhu2000 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuZhou-Joe Can't we simplify line 54 to line 62 like this? since we already set let branch = pkgBranch; outside the block and line 57 and line 61 are doing the duplicate thing

      const validDocPathsPattern = /^\d+\.\d+$/;
      const parsedBuildVersion = parse(buildVersion);
      
      if (!validDocPathsPattern.test(pkgBranch) &&& parsedBuildVersion) {
        branch = `${parsedBuildVersion.major}.${parsedBuildVersion.minor}`;
      } 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree and done.

Signed-off-by: suzhou <suzhou@amazon.com>
@abbyhu2000 abbyhu2000 merged commit e37ac4c into opensearch-project:main Mar 23, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 23, 2023
Add downgrade logic for branch in DocLinkService

Signed-off-by: suzhou <suzhou@amazon.com>
(cherry picked from commit e37ac4c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
joshuarrrr pushed a commit that referenced this pull request Mar 24, 2023
Add downgrade logic for branch in DocLinkService

Signed-off-by: suzhou <suzhou@amazon.com>
(cherry picked from commit e37ac4c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Mar 24, 2023
…3483)

Add downgrade logic for branch in DocLinkService

Signed-off-by: suzhou <suzhou@amazon.com>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…3483)

Add downgrade logic for branch in DocLinkService

Signed-off-by: suzhou <suzhou@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patch pkg.branch when build in CI flow
7 participants